From e77233e6d10763bed5abaa9c77007f4f873e3c72 Mon Sep 17 00:00:00 2001 From: Keir Fraser Date: Fri, 28 Mar 2008 17:50:10 +0000 Subject: [PATCH] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain(). In particular this *removes* some IS_PRIV_FOR() checks. *Especially* in particular, all domctls are executable only by dom0. Several of them were really unsafe for execution by a stub domain as they can affect global system resource usage. This probably breaks stub domains. Where necessary, some of these reversions can themselves be reverted where they are judged both necessary and safe. Signed-off-by: Keir Fraser --- xen/arch/x86/hvm/hvm.c | 11 +-- xen/arch/x86/mm.c | 28 +++++--- xen/common/domain.c | 2 +- xen/common/domctl.c | 142 +++++++++++-------------------------- xen/common/event_channel.c | 49 ++++++++----- xen/common/grant_table.c | 37 +++++----- xen/common/memory.c | 51 +++++++------ 7 files changed, 143 insertions(+), 177 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 00f6adb5c7..1f66fcb105 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2160,12 +2160,15 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg) return -EINVAL; if ( a.domid == DOMID_SELF ) + { d = rcu_lock_current_domain(); - else { - d = rcu_lock_domain_by_id(a.domid); - if ( d == NULL ) + } + else + { + if ( (d = rcu_lock_domain_by_id(a.domid)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rc = -EPERM; goto param_fail; } diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index bd92ec2f32..a1220af3b3 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2114,14 +2114,14 @@ static int set_foreigndom(domid_t domid) info->foreign = rcu_lock_domain(dom_xen); break; default: - e = rcu_lock_domain_by_id(domid); - if ( e == NULL ) + if ( (e = rcu_lock_domain_by_id(domid)) == NULL ) { MEM_LOG("Unknown domain '%u'", domid); okay = 0; break; } - if (!IS_PRIV_FOR(d, e)) { + if ( !IS_PRIV_FOR(d, e) ) + { MEM_LOG("Cannot set foreign dom"); okay = 0; rcu_unlock_domain(e); @@ -3259,12 +3259,15 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) return -EFAULT; if ( xatp.domid == DOMID_SELF ) + { d = rcu_lock_current_domain(); - else { - d = rcu_lock_domain_by_id(xatp.domid); - if ( d == NULL ) + } + else + { + if ( (d = rcu_lock_domain_by_id(xatp.domid)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rcu_unlock_domain(d); return -EPERM; } @@ -3355,12 +3358,15 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) return -EINVAL; if ( fmap.domid == DOMID_SELF ) + { d = rcu_lock_current_domain(); - else { - d = rcu_lock_domain_by_id(fmap.domid); - if ( d == NULL ) + } + else + { + if ( (d = rcu_lock_domain_by_id(fmap.domid)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rcu_unlock_domain(d); return -EPERM; } diff --git a/xen/common/domain.c b/xen/common/domain.c index 45a9de6a9c..76b48f4296 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -522,7 +522,7 @@ static void complete_domain_destroy(struct rcu_head *head) if ( (v = d->vcpu[i]) != NULL ) free_vcpu_struct(v); - if (d->target) + if ( d->target != NULL ) put_domain(d->target); free_domain(d); diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 2660af36d8..52143dbd1d 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -182,6 +182,9 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) struct xen_domctl curop, *op = &curop; static DEFINE_SPINLOCK(domctl_lock); + if ( !IS_PRIV(current->domain) ) + return -EPERM; + if ( copy_from_guest(op, u_domctl, 1) ) return -EFAULT; @@ -204,10 +207,6 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto svc_out; - ret = xsm_setvcpucontext(d); if ( ret ) goto svc_out; @@ -259,10 +258,6 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) ret = -ESRCH; if ( d != NULL ) { - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto pausedomain_out; - ret = xsm_pausedomain(d); if ( ret ) goto pausedomain_out; @@ -287,18 +282,16 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto unpausedomain_out; - ret = xsm_unpausedomain(d); if ( ret ) - goto unpausedomain_out; + { + rcu_unlock_domain(d); + break; + } domain_unpause_by_systemcontroller(d); - ret = 0; -unpausedomain_out: rcu_unlock_domain(d); + ret = 0; } break; @@ -310,18 +303,16 @@ unpausedomain_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto resumedomain_out; - ret = xsm_resumedomain(d); if ( ret ) - goto resumedomain_out; + { + rcu_unlock_domain(d); + break; + } domain_resume(d); - ret = 0; -resumedomain_out: rcu_unlock_domain(d); + ret = 0; } break; @@ -332,10 +323,6 @@ resumedomain_out: static domid_t rover = 0; unsigned int domcr_flags; - ret = -EPERM; - if ( !IS_PRIV(current->domain) ) - break; - ret = -EINVAL; if ( supervisor_mode_kernel || (op->u.createdomain.flags & @@ -401,13 +388,12 @@ resumedomain_out: if ( (d = rcu_lock_domain_by_id(op->domain)) == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto maxvcpu_out2; - ret = xsm_max_vcpus(d); if ( ret ) - goto maxvcpu_out2; + { + rcu_unlock_domain(d); + break; + } /* Needed, for example, to ensure writable p.t. state is synced. */ domain_pause(d); @@ -435,7 +421,6 @@ resumedomain_out: maxvcpu_out: domain_unpause(d); - maxvcpu_out2: rcu_unlock_domain(d); } break; @@ -446,9 +431,7 @@ resumedomain_out: ret = -ESRCH; if ( d != NULL ) { - ret = -EPERM; - if ( IS_PRIV_FOR(current->domain, d) ) - ret = xsm_destroydomain(d) ? : domain_kill(d); + ret = xsm_destroydomain(d) ? : domain_kill(d); rcu_unlock_domain(d); } } @@ -466,10 +449,6 @@ resumedomain_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto vcpuaffinity_out; - ret = xsm_vcpuaffinity(op->cmd, d); if ( ret ) goto vcpuaffinity_out; @@ -508,10 +487,6 @@ resumedomain_out: if ( (d = rcu_lock_domain_by_id(op->domain)) == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto scheduler_op_out; - ret = xsm_scheduler(d); if ( ret ) goto scheduler_op_out; @@ -533,7 +508,7 @@ resumedomain_out: rcu_read_lock(&domlist_read_lock); for_each_domain ( d ) - if ( d->domain_id >= dom && IS_PRIV_FOR(current->domain, d)) + if ( d->domain_id >= dom ) break; if ( d == NULL ) @@ -568,10 +543,6 @@ resumedomain_out: if ( (d = rcu_lock_domain_by_id(op->domain)) == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto getvcpucontext_out; - ret = xsm_getvcpucontext(d); if ( ret ) goto getvcpucontext_out; @@ -632,10 +603,6 @@ resumedomain_out: if ( (d = rcu_lock_domain_by_id(op->domain)) == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto getvcpuinfo_out; - ret = xsm_getvcpuinfo(d); if ( ret ) goto getvcpuinfo_out; @@ -675,10 +642,6 @@ resumedomain_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto max_mem_out; - ret = xsm_setdomainmaxmem(d); if ( ret ) goto max_mem_out; @@ -695,8 +658,6 @@ resumedomain_out: d->max_pages = new_max; ret = 0; } - else - printk("new max %ld, tot pages %d\n", new_max, d->tot_pages); spin_unlock(&d->page_alloc_lock); max_mem_out: @@ -713,19 +674,17 @@ resumedomain_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto setdomainhandle_out; - ret = xsm_setdomainhandle(d); if ( ret ) - goto setdomainhandle_out; + { + rcu_unlock_domain(d); + break; + } memcpy(d->handle, op->u.setdomainhandle.handle, sizeof(xen_domain_handle_t)); - ret = 0; -setdomainhandle_out: rcu_unlock_domain(d); + ret = 0; } break; @@ -738,20 +697,18 @@ setdomainhandle_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto setdebugging_out; - ret = xsm_setdebugging(d); if ( ret ) - goto setdebugging_out; + { + rcu_unlock_domain(d); + break; + } domain_pause(d); d->debugger_attached = !!op->u.setdebugging.enable; domain_unpause(d); /* causes guest to latch new status */ - ret = 0; -setdebugging_out: rcu_unlock_domain(d); + ret = 0; } break; @@ -769,10 +726,6 @@ setdebugging_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto irq_permission_out; - ret = xsm_irq_permission(d, pirq, op->u.irq_permission.allow_access); if ( ret ) goto irq_permission_out; @@ -802,10 +755,6 @@ setdebugging_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto iomem_permission_out; - ret = xsm_iomem_permission(d, mfn, op->u.iomem_permission.allow_access); if ( ret ) goto iomem_permission_out; @@ -829,19 +778,16 @@ setdebugging_out: if ( d == NULL ) break; - ret = -EPERM; - if ( !IS_PRIV_FOR(current->domain, d) ) - goto settimeoffset_out; - ret = xsm_domain_settime(d); if ( ret ) - goto settimeoffset_out; + { + rcu_unlock_domain(d); + break; + } d->time_offset_seconds = op->u.settimeoffset.time_offset_seconds; - - ret = 0; -settimeoffset_out: rcu_unlock_domain(d); + ret = 0; } break; @@ -854,32 +800,24 @@ settimeoffset_out: if ( d == NULL ) break; - ret = -EPERM; - if (!IS_PRIV_FOR(current->domain, d)) - goto set_target_out; - ret = -ESRCH; e = get_domain_by_id(op->u.set_target.target); if ( e == NULL ) goto set_target_out; - if ( d == e ) { - ret = -EINVAL; - put_domain(e); - goto set_target_out; - } - - if (!IS_PRIV_FOR(current->domain, e)) { - ret = -EPERM; + ret = -EINVAL; + if ( (d == e) || (d->target != NULL) ) + { put_domain(e); goto set_target_out; } + /* Hold reference on @e until we destroy @d. */ d->target = e; - /* and we keep the reference on e, released when destroying d */ + ret = 0; -set_target_out: + set_target_out: rcu_unlock_domain(d); } break; diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 365adf4652..b385b54738 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -130,13 +130,17 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) long rc; if ( dom == DOMID_SELF ) - d = current->domain; - else { + { + d = rcu_lock_current_domain(); + } + else + { if ( (d = rcu_lock_domain_by_id(dom)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { - rc = -EPERM; - goto out2; + if ( !IS_PRIV_FOR(current->domain, d) ) + { + rcu_unlock_domain(d); + return -EPERM; } } @@ -158,8 +162,6 @@ static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) out: spin_unlock(&d->evtchn_lock); - - out2: rcu_unlock_domain(d); return rc; @@ -201,7 +203,7 @@ static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) ERROR_EXIT_DOM(-EINVAL, rd); rchn = evtchn_from_port(rd, rport); if ( (rchn->state != ECS_UNBOUND) || - (rchn->u.unbound.remote_domid != ld->domain_id && !IS_PRIV_FOR(ld, rd))) + (rchn->u.unbound.remote_domid != ld->domain_id) ) ERROR_EXIT_DOM(-EINVAL, rd); rc = xsm_evtchn_interdomain(ld, lchn, rd, rchn); @@ -631,13 +633,17 @@ static long evtchn_status(evtchn_status_t *status) long rc = 0; if ( dom == DOMID_SELF ) - d = current->domain; - else { + { + d = rcu_lock_current_domain(); + } + else + { if ( (d = rcu_lock_domain_by_id(dom)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { - rc = -EPERM; - goto out2; + if ( !IS_PRIV_FOR(current->domain, d) ) + { + rcu_unlock_domain(d); + return -EPERM; } } @@ -690,8 +696,8 @@ static long evtchn_status(evtchn_status_t *status) out: spin_unlock(&d->evtchn_lock); - out2: rcu_unlock_domain(d); + return rc; } @@ -742,6 +748,7 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id) out: spin_unlock(&d->evtchn_lock); + return rc; } @@ -784,15 +791,18 @@ static long evtchn_reset(evtchn_reset_t *r) { domid_t dom = r->dom; struct domain *d; - int i; - int rc; + int i, rc; if ( dom == DOMID_SELF ) - d = current->domain; - else { + { + d = rcu_lock_current_domain(); + } + else + { if ( (d = rcu_lock_domain_by_id(dom)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rc = -EPERM; goto out; } @@ -806,6 +816,7 @@ static long evtchn_reset(evtchn_reset_t *r) (void)__evtchn_close(d, i); rc = 0; + out: rcu_unlock_domain(d); diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 3f69e822fe..2dbcfab8b6 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -828,32 +828,34 @@ gnttab_setup_table( " per domain.\n", max_nr_grant_frames); op.status = GNTST_general_error; - goto out; + goto out1; } dom = op.dom; if ( dom == DOMID_SELF ) { - d = current->domain; + d = rcu_lock_current_domain(); } - else { + else + { if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) ) { gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom); op.status = GNTST_bad_domain; - goto out; + goto out1; } - if ( unlikely(!IS_PRIV_FOR(current->domain, d)) ) { + + if ( unlikely(!IS_PRIV_FOR(current->domain, d)) ) + { op.status = GNTST_permission_denied; - goto setup_unlock_out2; + goto out2; } } if ( xsm_grant_setup(current->domain, d) ) { - rcu_unlock_domain(d); op.status = GNTST_permission_denied; - goto out; + goto out2; } spin_lock(&d->grant_table->lock); @@ -867,7 +869,7 @@ gnttab_setup_table( nr_grant_frames(d->grant_table), max_nr_grant_frames); op.status = GNTST_general_error; - goto setup_unlock_out; + goto out3; } op.status = GNTST_okay; @@ -877,13 +879,11 @@ gnttab_setup_table( (void)copy_to_guest_offset(op.frame_list, i, &gmfn, 1); } - setup_unlock_out: + out3: spin_unlock(&d->grant_table->lock); - - setup_unlock_out2: + out2: rcu_unlock_domain(d); - - out: + out1: if ( unlikely(copy_to_guest(uop, &op, 1)) ) return -EFAULT; @@ -911,16 +911,19 @@ gnttab_query_size( dom = op.dom; if ( dom == DOMID_SELF ) { - d = current->domain; + d = rcu_lock_current_domain(); } - else { + else + { if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) ) { gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom); op.status = GNTST_bad_domain; goto query_out; } - if ( unlikely(!IS_PRIV_FOR(current->domain, d)) ) { + + if ( unlikely(!IS_PRIV_FOR(current->domain, d)) ) + { op.status = GNTST_permission_denied; goto query_out_unlock; } diff --git a/xen/common/memory.c b/xen/common/memory.c index a3c2ad65e6..70a05d5367 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -232,12 +232,15 @@ static long translate_gpfn_list( return -EFAULT; if ( op.domid == DOMID_SELF ) - d = current->domain; - else { - d = rcu_lock_domain_by_id(op.domid); - if ( d == NULL ) + { + d = rcu_lock_current_domain(); + } + else + { + if ( (d = rcu_lock_domain_by_id(op.domid)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rcu_unlock_domain(d); return -EPERM; } @@ -539,12 +542,15 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE(void) arg) } if ( likely(reservation.domid == DOMID_SELF) ) - d = current->domain; - else { - d = rcu_lock_domain_by_id(reservation.domid); - if ( d == NULL) + { + d = rcu_lock_current_domain(); + } + else + { + if ( (d = rcu_lock_domain_by_id(reservation.domid)) == NULL ) return start_extent; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rcu_unlock_domain(d); return start_extent; } @@ -554,8 +560,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE(void) arg) rc = xsm_memory_adjust_reservation(current->domain, d); if ( rc ) { - if ( reservation.domid != DOMID_SELF ) - rcu_unlock_domain(d); + rcu_unlock_domain(d); return rc; } @@ -572,8 +577,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE(void) arg) break; } - if ( unlikely(reservation.domid != DOMID_SELF) ) - rcu_unlock_domain(d); + rcu_unlock_domain(d); rc = args.nr_done; @@ -599,12 +603,15 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE(void) arg) return -EFAULT; if ( likely(domid == DOMID_SELF) ) - d = current->domain; - else { - d = rcu_lock_domain_by_id(domid); - if ( d == NULL ) + { + d = rcu_lock_current_domain(); + } + else + { + if ( (d = rcu_lock_domain_by_id(domid)) == NULL ) return -ESRCH; - if ( !IS_PRIV_FOR(current->domain, d) ) { + if ( !IS_PRIV_FOR(current->domain, d) ) + { rcu_unlock_domain(d); return -EPERM; } @@ -613,8 +620,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE(void) arg) rc = xsm_memory_stat_reservation(current->domain, d); if ( rc ) { - if ( domid != DOMID_SELF ) - rcu_unlock_domain(d); + rcu_unlock_domain(d); return rc; } @@ -632,8 +638,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE(void) arg) break; } - if ( unlikely(domid != DOMID_SELF) ) - rcu_unlock_domain(d); + rcu_unlock_domain(d); break; -- 2.30.2